Skip to content

added default time window#5530

Merged
chitalian merged 2 commits intomainfrom
justin/default-time-window
Jan 21, 2026
Merged

added default time window#5530
chitalian merged 2 commits intomainfrom
justin/default-time-window

Conversation

@chitalian
Copy link
Contributor

Ticket

Link to the ticket(s) this pull request addresses.

Component/Service

What part of Helicone does this affect?

  • Web (Frontend)
  • Jawn (Backend)
  • Worker (Proxy)
  • Bifrost (Marketing)
  • AI Gateway
  • Packages
  • Infrastructure/Docker
  • Documentation

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Performance improvement
  • Refactoring

Deployment Notes

  • No special deployment steps required
  • Database migrations need to run
  • Environment variable changes required
  • Coordination with other teams needed

Screenshots / Demos

Before After

Extra Notes

Any additional context, considerations, or notes for reviewers.

Context

Why are you making this change?

Screenshots / Demos

@vercel
Copy link

vercel bot commented Jan 16, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
helicone Ready Ready Preview, Comment Jan 21, 2026 4:18am
helicone-eu Ready Ready Preview, Comment Jan 21, 2026 4:18am
1 Skipped Deployment
Project Deployment Review Updated (UTC)
helicone-bifrost Skipped Skipped Jan 21, 2026 4:18am

Request Review

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@supabase
Copy link

supabase bot commented Jan 16, 2026

This pull request has been ignored for the connected project bolqqmqbrciybnypvklh because there are no changes detected in supbase/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@supabase
Copy link

supabase bot commented Jan 16, 2026

This pull request has been ignored for the connected project lmahfbbnchpworytrrqk because there are no changes detected in .supabase/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@claude
Copy link
Contributor

claude bot commented Jan 16, 2026

Claude finished @chitalian's task —— View job


PR Review: Default Time Window Feature

Reviewing this PR for code quality, security, performance, and best practices.

Review Checklist:

  • Analyze database migration for schema changes and potential issues
  • Review backend API changes and data handling
  • Examine frontend implementation and UI components
  • Check type definitions and consistency across services
  • Identify security vulnerabilities or data exposure risks
  • Look for performance optimizations and potential bottlenecks
  • Verify code follows repository best practices
  • Provide actionable feedback with specific suggestions
  • Assign confidence score and summary

Code Review Findings

Database Migration

File: supabase/migrations/20260116133358_default_time_filter.sql

  • Safe migration using IF NOT EXISTS to prevent conflicts
  • Proper default value of '7d' aligns with existing UI patterns
  • No breaking changes or data loss concerns

Backend Implementation

Files: OrganizationStore.ts, OrganizationManager.ts

Strengths:

  • Proper authorization checks in updateOrganization() method
  • Follows existing patterns with parameterized queries
  • Includes the new field in UpdateOrganizationParams type consistently

Frontend Implementation

Files: orgSettingsPage.tsx, dashboardPage.tsx, RequestsPage.tsx

Strengths:

  • Clean UI implementation with proper form handling
  • Consistent with design system (using SelectValue, proper labels)
  • Good user experience with immediate updates via handleOrgUpdate
  • Proper fallback handling (orgContext?.currentOrg?.default_time_filter ?? "7d")

🔧 Areas for Improvement

1. Input Validation (Medium Priority)

Issue: No validation for time filter values
Files: OrganizationStore.ts:150-154, orgSettingsPage.tsx:225-236

// Current - no validation
if (updateOrgParams.default_time_filter !== undefined) {
  updateFields.push(`default_time_filter = $${paramIndex}`);
  params.push(updateOrgParams.default_time_filter);
  paramIndex++;
}

Suggestion: Add validation to prevent invalid time filter values:

const VALID_TIME_FILTERS = ['1h', '24h', '7d', '1m', '3m'] as const;

if (updateOrgParams.default_time_filter !== undefined) {
  if (!VALID_TIME_FILTERS.includes(updateOrgParams.default_time_filter as any)) {
    return err("Invalid time filter value");
  }
  updateFields.push(`default_time_filter = $${paramIndex}`);
  params.push(updateOrgParams.default_time_filter);
  paramIndex++;
}

2. Import Optimization (Low Priority)

File: dashboardPage.tsx:34

// Current - missing import for getTimeIntervalAgo
const defaultTimeFilter = (orgContext?.currentOrg?.default_time_filter ?? "7d") as TimeInterval;

The code uses getTimeIntervalAgo but it should be imported from the time calculations module.

3. TypeScript Consistency (Low Priority)

Files: Type definitions could be more explicit about the time filter type instead of using string.

type TimeFilterOption = '1h' | '24h' | '7d' | '1m' | '3m';

🚀 Performance & UX

  • Efficient debounced updates (500ms) prevent excessive API calls
  • Proper React Query cache invalidation maintains data consistency
  • Good user feedback with success notifications

🔒 Security

  • Proper authorization checks in backend (checkAccessToMutateOrg)
  • Role-based access (admin/owner required for updates)
  • No SQL injection risks with parameterized queries
  • No sensitive data exposure

📋 Code Quality

  • Follows repository patterns consistently
  • Proper error handling with Result types
  • Good separation of concerns (store/manager/UI)
  • Adheres to design system guidelines

Score: 7/10 - Good implementation with minor improvements suggested

Suggestions Summary:

  • Critical: None - code is safe to merge
  • Recommended: Add input validation for time filter values
  • Optional: Improve import organization and type definitions
  • Positive: Well-structured feature that follows existing patterns

The implementation is solid and follows the codebase's established patterns well. The feature adds genuine value without introducing security risks or breaking changes.

The mutation was missing the default_time_filter parameter, so changes
to the default time window setting were never being sent to the API.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@vercel vercel bot temporarily deployed to Preview – helicone-bifrost January 21, 2026 04:14 Inactive
@chitalian chitalian merged commit b46ea6f into main Jan 21, 2026
9 checks passed
@chitalian chitalian deleted the justin/default-time-window branch January 21, 2026 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant